Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Convert createWasm async/await where possible #23157

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Convert createWasm async/await where possible #23157

merged 1 commit into from
Dec 19, 2024

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 13, 2024 •
edited
Loading

The advantage if using await in the cases where we can is that it avoids the generation or wrapper function for each export.

So instead of

var wasmExport = createWasm(); // returns empty object
...
var malloc = (..) => (malloc = wasmExports['malloc'])(..);

We can just generate:

var wasmExport = await createWasm(); // returns actual exports
...
var malloc = wasmExports['malloc'];

This only currently works in MODULARIZE mode where the code is running inside a factory function. Otherwise it would end up using top-level-await.

One wrinkle here is that this is not currently supported when closure is enabled because we run closure only on the contents of the factory function so closure ends up seeing this as a top level await when its not.

There are two minor observable effects of this change:

  1. In MODULARIZE mode its no longer possible to call new Foo() with factory function. We already added a warning for this debug builds in Assert if MODULARIZE factory function is used with new. #23210.
  2. Because we now can await for createWasm to return, the run method can run on first call, which means it runs main on first call, which means main will now run before postjs code, just like it would with sync instantiation.

sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 16, 2024
@sbc100
Don't inject assertions onto the ready promise
e370f04 
This is a debug feature that means users who did not await the promise
and tried to access stuff directly on it would see a nice error message.

However with the advent of more usage of async/await, for example in
\emscripten-core#23157 it is not always the ready promise that gets returned to the
user but a language-generated promise from an `await` call.

We could try to jump through more hoops to continue to support this
debug feature, but I don't think added complexity in the toolchain
or the generated code would be worth it.

Split out from emscripten-core#23157.
@sbc100 sbc100 mentioned this pull request Dec 16, 2024
Merged
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 16, 2024
@sbc100
Don't inject assertions onto the ready promise
1958e67 
This is a debug feature that means users who did not await the promise
and tried to access stuff directly on it would see a nice error message.

However with the advent of more usage of async/await, for example in
\emscripten-core#23157 it is not always the ready promise that gets returned to the
user but a language-generated promise from an `await` call.

We could try to jump through more hoops to continue to support this
debug feature, but I don't think added complexity in the toolchain
or the generated code would be worth it.

Split out from emscripten-core#23157.
sbc100 added a commit that referenced this pull request Dec 16, 2024
@sbc100
Don't inject assertions onto the ready promise (#23176)
df004b2 
This is a debug feature that means users who did not await the promise
and tried to access stuff directly on it would see a nice error message.

However with the advent of more usage of async/await, for example in
\#23157 it is not always the ready promise that gets returned to the
user but a language-generated promise from an `await` call.

We could try to jump through more hoops to continue to support this
debug feature, but I don't think added complexity in the toolchain or
the generated code would be worth it.

Split out from #23157.
@sbc100 sbc100 force-pushed the await_createWasm branch 6 times, most recently from b85a441 to 3106ca5 Compare December 17, 2024 21:10
sbc100
sbc100 commented Dec 17, 2024
View reviewed changes
test/test_browser.py Outdated Show resolved Hide resolved
@sbc100 sbc100 requested a review from dschuff December 17, 2024 22:26
@sbc100 sbc100 changed the title Allow the createWasm function to use async/await where possible Convert createWasm async/await where possible Dec 17, 2024
@sbc100 sbc100 changed the title Convert createWasm async/await where possible Convert createWasm async/await where possible Dec 17, 2024
kripken
kripken reviewed Dec 17, 2024
View reviewed changes
test/test_browser.py Outdated Show resolved Hide resolved
tools/emscripten.py Show resolved Hide resolved
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
9e34723 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
@sbc100 sbc100 mentioned this pull request Dec 18, 2024
Merged
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
112b953 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
7aca82a 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
f04ba5a 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
4f32743 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
hedwigz pushed a commit to hedwigz/emscripten that referenced this pull request Dec 18, 2024
@sbc100 @hedwigz
Don't inject assertions onto the ready promise (emscripten-core#23176)
b81bba5 
This is a debug feature that means users who did not await the promise
and tried to access stuff directly on it would see a nice error message.

However with the advent of more usage of async/await, for example in
\emscripten-core#23157 it is not always the ready promise that gets returned to the
user but a language-generated promise from an `await` call.

We could try to jump through more hoops to continue to support this
debug feature, but I don't think added complexity in the toolchain or
the generated code would be worth it.

Split out from emscripten-core#23157.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
02a1a96 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
15efbd0 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
cc98d3b 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
ae674ad 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
ced299a 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit to sbc100/emscripten that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new.
1a2ee30 
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
sbc100 added a commit that referenced this pull request Dec 18, 2024
@sbc100
Assert if MODULARIZE factory function is used with new. (#23210)
669e01a 
Once we start using `async` for for this function then `new` stops
working: #23157.

Using `new` in this was was always an antipattern but there was nothing
atopping users from doing this.
@sbc100 sbc100 force-pushed the await_createWasm branch 2 times, most recently from a7bb4fa to 9e0033a Compare December 18, 2024 22:59
@sbc100 sbc100 requested a review from kripken December 18, 2024 23:00
kripken
kripken reviewed Dec 18, 2024
View reviewed changes
tools/emscripten.py Outdated Show resolved Hide resolved
tools/emscripten.py Outdated Show resolved Hide resolved
ChangeLog.md Outdated Show resolved Hide resolved
@sbc100 sbc100 requested a review from kripken December 19, 2024 00:41
@sbc100
Allow the createWasm function to use async/await where possible
ce417c6 
The advantage if using `await` in the cases where we can is that it
avoids the generation or wrapper function for each export.

So instead of

```
var wasmExport = createWasm(); // returns empty object
...
var malloc = ((..) => (malloc = wasmExports['malloc'])()
```

We can just generate:

```
var wasmExport = createWasm(); // returns empty object
...
var malloc = wasmExports['malloc'];
```

This only currently works in MODULARIZE mode where the code is running
inside a factory function.  Otherwise it would end up using
top-level-await.

One wrinkle here is that this is not currently supported when closure is
enabled because we run closure only on the contents of the factory
function so closure ends up seeing this as a top level await when its
not.
kripken
kripken reviewed Dec 19, 2024
View reviewed changes
ChangeLog.md Outdated Show resolved Hide resolved
@sbc100 sbc100 merged commit b330159 into emscripten-core:main Dec 19, 2024
7 of 14 checks passed
@sbc100 sbc100 deleted the await_createWasm branch December 19, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken left review comments

@brendandahl brendandahl Awaiting requested review from brendandahl

@dschuff dschuff Awaiting requested review from dschuff

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @kripken